Reject future blocks #492

teor2345 · 2020-06-16T13:38:40Z

Reject blocks that are more than 2 hours in the future, according to the node's clock.

~~I'll be able to complete this PR, once we've finished the async AddBlock() changes in PR #491.~~

Edit: PR #491 wasn't actually needed.

codecov · 2020-06-16T13:49:45Z

Codecov Report

Merging #492 into main will increase coverage by 0.37%.
The diff coverage is 48.28%.

@@            Coverage Diff             @@
##             main     #492      +/-   ##
==========================================
+ Coverage   53.05%   53.42%   +0.37%     
==========================================
  Files          68       80      +12     
  Lines        3212     3942     +730     
==========================================
+ Hits         1704     2106     +402     
- Misses       1508     1836     +328

Impacted Files	Coverage Δ
tower-batch/src/error.rs	`0.00% <0.00%> (ø)`
tower-batch/src/layer.rs	`0.00% <0.00%> (ø)`
zebra-chain/src/keys/sapling.rs	`85.30% <ø> (ø)`
zebra-chain/src/lib.rs	`50.00% <ø> (ø)`
zebra-chain/src/merkle_tree.rs	`0.00% <ø> (ø)`
zebra-chain/src/note_commitment_tree.rs	`0.00% <ø> (ø)`
zebra-chain/src/serde_helpers.rs	`0.00% <0.00%> (ø)`
zebra-chain/src/transaction.rs	`16.66% <ø> (+16.66%)`	⬆️
zebra-chain/src/transaction/transparent.rs	`100.00% <ø> (ø)`
zebra-network/src/peer/connection.rs	`0.00% <0.00%> (ø)`
... and 75 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6771889...20dacb9. Read the comment docs.

teor2345 · 2020-06-16T13:56:43Z

In this case, the time check is a quick check, so we can just do it in call(), and if it fails, return async { Err() } as the Future.

But we'll still need PR #491 to do expensive checks in the async block.

hdevalence

👍

dconnolly

Writing a test to cover this seems annoying to set up so I'm approving without a test, but we may want to create an issue to make one and make it open to contributors / good-first-issue.

dconnolly · 2020-06-16T21:01:09Z

zebra-consensus/src/verify.rs

+    /// Returns true if the block header time is less than or equal to
+    /// 2 hours in the future, according to the node's local clock.
+    ///
+    /// This is a non-deterministic rule, as clocks vary over time, and


dconnolly · 2020-06-16T21:01:24Z

zebra-consensus/src/verify.rs

+    /// This is a non-deterministic rule, as clocks vary over time, and
+    /// between different nodes.
+    ///
+    /// "In addition, a full validator MUST NOT accept blocks with nTime


👍👍👍

teor2345 · 2020-06-17T00:03:58Z

Writing a test to cover this seems annoying to set up so I'm approving without a test, but we may want to create an issue to make one and make it open to contributors / good-first-issue.

I think a test for "2 hours in the future" should be reasonably easy, let me see if I can set one up.

It will be harder to test overflow, I might need to refactor to do that.

teor2345 · 2020-06-17T11:47:29Z

I rebased this branch to handle the conflicts with the comments I cherry-picked from PR #491.

Writing a test to cover this seems annoying to set up so I'm approving without a test, but we may want to create an issue to make one and make it open to contributors / good-first-issue.

I think a test for "2 hours in the future" should be reasonably easy, let me see if I can set one up.

It will be harder to test overflow, I might need to refactor to do that.

I tested:

2 hours in the future,
2 hours and 1 second in the future,
a bunch of nearby times,
an old block, and
a bunch of significant time constants.

Utc.timestamp() panics when the timestamp is out of range. Its valid range is smaller than i64, so I don't think overflow is possible. (But if it is, the check handles it correctly, by returning an error.)

The Utc.timestamp() valid range is larger than the Zcash time range (u32), so all Zcash times are valid in Rust. I've made a note in #477 to check for overflows when adding times from multiple blocks.

teor2345 · 2020-06-18T11:34:35Z

I've set this PR to draft, because I need to fix an issue with early state commits, as discussed here:
#502 (comment)

I should also add some tests, to make sure that the state isn't changed on verification errors.

teor2345 · 2020-06-20T00:37:25Z

zebra-consensus/src/verify.rs

+        //
+        // `tower::Buffer` expects exactly one `call` for each
+        // `poll_ready`. So we unconditionally create the AddBlock
+        // Future using `state_service.call`. If verification fails,
+        // we return an error, and implicitly cancel the future.
+        let add_block = self.state_service.call(zebra_state::Request::AddBlock {
+            block: block.clone(),
+        });

        async move {
+            // If verification fails, return an error result.
+            // The AddBlock Future is implicitly cancelled by the
+            // error return in `?`.
+
+            // Since errors cause an early exit, try to do the
+            // quick checks first.
+            block::node_time_check(block)?;
+
+            // Verification was successful.
+            // Add the block to the state by awaiting the AddBlock
+            // Future, and return its result.
            match add_block.await? {
                zebra_state::Response::Added { hash } => Ok(hash),
                _ => Err("adding block to zebra-state failed".into()),


As discussed in #502, we can't conditionally await an AddBlock future. Because the state service can commit changes in call(), or in the Future's async block (that is, in await).

Fixed in 589f1ce.

zebra-chain/src/block.rs

yaahc · 2020-06-21T16:59:26Z

zebra-consensus/src/verify/block.rs

+///
+/// [7.5]: https://zips.z.cash/protocol/protocol.pdf#blockheader
+pub(super) fn node_time_check(block: Arc<Block>) -> Result<(), Error> {
+    node_time_check_helper(block.header.time, Utc::now())


TIOLI: I don't think this benefits from being broken out into a separate function, I'd prefer to start the fn with

let block_header_time = block.header.time; let now = Utc::now(); // The rest of node_time_check_helper's function body...

You're right, it made more sense before I split out the node_time_check_helper() for testing.

Fixed in 347f5ba.

teor2345 · 2020-06-22T02:44:21Z

I think we can let GitHub squash and merge this PR.

(I could do a manual squash into a time check commit, a BlockVerifier refactor commit, and other changes. Happy to do that if people think it's worth the effort.)

dconnolly

All these tests fill me with joy ☺️

zebra-consensus/src/verify.rs

yaahc · 2020-06-22T17:20:09Z

zebra-consensus/src/verify.rs

-                // TODO(teor || jlusby): check error string
-                _ => true,
-            },
+            // TODO(teor || jlusby): check error string


I wouldn't default to doing string comparisons, if there's ever a specific error that we need to check for that is being constructed from str -> Box<dyn Error> we should instead change the error to be a proper error type that we can downcast to, using string's content as a proxy for a typed error is a recipe for disaster.

I'll fix these comments, possibly in a future PR.

Fixed in #538 in 5f96b32 for verify/block

Fixed in #538 in 609de23 for checkpoint

zebra-consensus/src/verify.rs

hdevalence

TIOLI: I think the BlockVerifier belongs in the block submodule, and generally I think we should keep all of the contents of the code in the leaves of the module tree and then re-export as necessary.

dconnolly · 2020-06-23T04:14:37Z

Looks like this needs a rebase but LGTM! 💃

We don't need to check transaction sizes yet, because we aren't parsing or generating transactions outside of blocks. Part of ZcashFoundation#483.

Add a function that checks the block header miner times against the node's local clock. (We'll use this function in the next commit.) Part of ZcashFoundation#477.

Part of ZcashFoundation#477.

Use MAX constants for the block header version and time arbitrary test ranges. Reduces the block header time arbitrary test range from 2**32 to 2**32-1 (u32::MAX). (2**32 is an invalid time value, which gets truncated to 0 during serialization.) Also add some comments about DateTime conversions. Part of ZcashFoundation#477.

Part of ZcashFoundation#477.

We don't want to call the state's AddBlock until we know the block is valid. This avoids subtle bugs if the state is modified in call(). (in_memory currently modifies the state in call(), on_disk modifies the state in the async block.) Part of ZcashFoundation#477.

node_time_check() is a small function, so we inline it into its callers. (And then rename node_time_check_helper() to node_time_check().) Part of ZcashFoundation#477.

Part of ZcashFoundation#477.

teor2345 · 2020-06-24T07:03:18Z

I rebased and made the following changes:

zebra-test module name change
color-eyre/eyre module import changes

I'll merge after CI passes, I don't think the differences are large enough for a review.

teor2345 · 2020-06-24T08:22:41Z

The checks passed, and the changes are minor, so I merged this PR.

teor2345 · 2020-06-24T08:27:57Z

TIOLI: I think the BlockVerifier belongs in the block submodule, and generally I think we should keep all of the contents of the code in the leaves of the module tree and then re-export as necessary.

Fixed in #538 in e2999fc.

teor2345 added A-dependencies Area: Dependency file updates Poll::Ready A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code labels Jun 16, 2020

teor2345 added this to the Validate transactions. milestone Jun 16, 2020

teor2345 requested a review from a team June 16, 2020 13:38

teor2345 self-assigned this Jun 16, 2020

teor2345 mentioned this pull request Jun 16, 2020

Tracking: block header validation #477

Closed

16 tasks

hdevalence previously approved these changes Jun 16, 2020

View reviewed changes

dconnolly previously approved these changes Jun 16, 2020

View reviewed changes

teor2345 mentioned this pull request Jun 17, 2020

WIP: Try to do block verification async #491

Closed

teor2345 dismissed stale reviews from dconnolly and hdevalence via aa98871 June 17, 2020 11:25

teor2345 force-pushed the reject-future-blocks branch from 9957d0c to aa98871 Compare June 17, 2020 11:25

teor2345 changed the title ~~WIP: Reject future blocks~~ Reject future blocks Jun 17, 2020

teor2345 requested review from hdevalence and dconnolly June 18, 2020 07:54

teor2345 marked this pull request as draft June 18, 2020 11:33

teor2345 commented Jun 20, 2020

View reviewed changes

yaahc reviewed Jun 21, 2020

View reviewed changes

teor2345 marked this pull request as ready for review June 22, 2020 00:14

teor2345 requested a review from yaahc June 22, 2020 00:14

dconnolly previously approved these changes Jun 22, 2020

View reviewed changes

zebra-consensus/src/verify.rs Show resolved Hide resolved

zebra-consensus/src/verify.rs Show resolved Hide resolved

yaahc reviewed Jun 22, 2020

View reviewed changes

hdevalence previously approved these changes Jun 22, 2020

View reviewed changes

teor2345 added 11 commits June 24, 2020 16:48

chain: Add a comment about transaction sizes

bf88680

We don't need to check transaction sizes yet, because we aren't parsing or generating transactions outside of blocks. Part of ZcashFoundation#483.

consensus: Add a block time check function

4342abd

Add a function that checks the block header miner times against the node's local clock. (We'll use this function in the next commit.) Part of ZcashFoundation#477.

consensus: Check block times against the local clock

4cea5bf

Part of ZcashFoundation#477.

consensus: Refactor the node time check for testing

b3e95d2

Part of ZcashFoundation#477.

consensus: move verify::block to its own file

087d4c1

Part of ZcashFoundation#477.

consensus: Add tests for block node time checks

69ce8cc

Part of ZcashFoundation#477.

consensus: Inline node_time_check() into callers

6708f29

node_time_check() is a small function, so we inline it into its callers. (And then rename node_time_check_helper() to node_time_check().) Part of ZcashFoundation#477.

consensus: Simplify error handling in a test

f2629b5

Part of ZcashFoundation#477.

chain: Update the note about time truncation

20dacb9

teor2345 dismissed stale reviews from hdevalence and dconnolly via 20dacb9 June 24, 2020 07:01

teor2345 force-pushed the reject-future-blocks branch from 153a968 to 20dacb9 Compare June 24, 2020 07:01

teor2345 merged commit a9efb87 into ZcashFoundation:main Jun 24, 2020

teor2345 mentioned this pull request Jun 24, 2020

Consensus test tweaks #538

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reject future blocks #492

Reject future blocks #492

teor2345 commented Jun 16, 2020 •

edited

Loading

codecov bot commented Jun 16, 2020 •

edited

Loading

teor2345 commented Jun 16, 2020

hdevalence left a comment

dconnolly left a comment

dconnolly Jun 16, 2020

dconnolly Jun 16, 2020

teor2345 commented Jun 17, 2020

teor2345 commented Jun 17, 2020

teor2345 commented Jun 18, 2020

teor2345 Jun 20, 2020

teor2345 Jun 22, 2020

yaahc Jun 21, 2020

teor2345 Jun 21, 2020

teor2345 Jun 22, 2020

teor2345 commented Jun 22, 2020

dconnolly left a comment

yaahc Jun 22, 2020 •

edited

Loading

teor2345 Jun 22, 2020

teor2345 Jun 24, 2020

teor2345 Jun 24, 2020

hdevalence left a comment

dconnolly commented Jun 23, 2020

teor2345 commented Jun 24, 2020

teor2345 commented Jun 24, 2020

teor2345 commented Jun 24, 2020

Reject future blocks #492

Reject future blocks #492

Conversation

teor2345 commented Jun 16, 2020 • edited Loading

codecov bot commented Jun 16, 2020 • edited Loading

Codecov Report

teor2345 commented Jun 16, 2020

hdevalence left a comment

Choose a reason for hiding this comment

dconnolly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 commented Jun 17, 2020

teor2345 commented Jun 17, 2020

teor2345 commented Jun 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 commented Jun 22, 2020

dconnolly left a comment

Choose a reason for hiding this comment

yaahc Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdevalence left a comment

Choose a reason for hiding this comment

dconnolly commented Jun 23, 2020

teor2345 commented Jun 24, 2020

teor2345 commented Jun 24, 2020

teor2345 commented Jun 24, 2020

teor2345 commented Jun 16, 2020 •

edited

Loading

codecov bot commented Jun 16, 2020 •

edited

Loading

yaahc Jun 22, 2020 •

edited

Loading